-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: display skip reason in new ui #618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
@@ -109,6 +110,23 @@ function MetaInfoInternal(props: MetaInfoInternalProps): ReactNode { | |||
}); | |||
} | |||
|
|||
const shouldAddSkipReason = Boolean(!metaInfoItemsWithResolvedUrls.find(item => item.label === 'muteReason')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use a separate variable for this expression - Boolean(!metaInfoItemsWithResolvedUrls.find(item => item.label === 'muteReason'))
. For example - hasMuteReason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this with sipayrt and decided that it's not very good idea to include such specific logic related to muteReason
, so i got rid of altogether
content: reason.props.children, | ||
url: reason.props.href | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can reason
be something else? I mean is there a condition under which the value does not fall into any of the described conditions (if
and else if
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is no longer here.
label: 'skipReason', | ||
content: reason | ||
}); | ||
} else if (!Array.isArray(reason) && typeof reason.props.children === 'string' && typeof reason.props.href === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason can be an array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer here, got rid of parsing html entirely.
if (props.item.images?.length) { | ||
if (props.item.skipReason) { | ||
return <div className={styles.skipReasonContainer}> | ||
<div className={styles.skipReason}>Skipped ⋅ {Parser(props.item.skipReason)}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⋅
- why you choose a dot as a separator? But in IU it looks quite harmonious. Maybe you use it in some other components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks nice as a separator and it's not the only place we use dot. The other places are hint toast, and other variants of subtitle in tree view.
1018dba
to
dd1119d
Compare
What's done:
Demo report: https://nda.ya.ru/t/Ba_4zPC979ttwt